Repository navigation
Support percentage-weighted choices in custom YAML providers - #1962
Sinkleberg wants to merge 1 commit into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1962 +/- ##
============================================
- Coverage 92.50% 92.45% -0.05%
- Complexity 3583 3600 +17
============================================
Files 348 349 +1
Lines 7081 7146 +65
Branches 694 711 +17
============================================
+ Hits 6550 6607 +57
- Misses 369 370 +1
- Partials 162 169 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
asolntsev
left a comment
asolntsev
left a comment
There was a problem hiding this comment.
Thanks, this is a useful feature and the tests are thorough. Two things I'd like fixed before merging: the Random shim in select() (with the per-call re-validation), and error messages that don't say which key or file is broken. The other comments are minor.
Design question: requiring the percentages to add up to exactly 100 is clear for users, but it rules out relative weights like 3 and 1. That's fine for a first version, but it's worth deciding now whether the syntax should later accept plain numbers too, so it doesn't have to change.
| return new WeightedYamlValues(choices); | ||
| } | ||
|
|
||
| String select(RandomService random) { |
There was a problem hiding this comment.
This subclasses new Random(0) and overrides only nextDouble(). That works only as long as WeightedRandomSelector never calls any other Random method. If it ever does, values would silently come from a stream seeded with 0. Each call also runs WeightedRandomSelector.select, which rebuilds a HashSet, re-checks every item and recomputes the cumulative weights on every resolve, although the data was already validated at load time.
Suggestion: compute double[] cumulative and Object[] values once in from(), then:
Object selected = WeightedRandomSelector.selectWeightedElement(random.nextDouble() * total, cumulative, values);(selectWeightedElement is already package-private and static.) This removes the shim and the per-call overhead.
| return (String) selected; | ||
| } | ||
|
|
||
| private static IllegalArgumentException invalid(String reason) { |
There was a problem hiding this comment.
The error doesn't say which key or file is broken. Values are loaded lazily, one whole file at a time, so Invalid weighted YAML choices: percentages must total 100 appears on the first lookup of any key in the file, often far from the bad entry. Could you pass the key path (and URL) into from(), or wrap the exception in FakeValues.loadFromUrl and add them there?
| } | ||
| BigDecimal percentage = new BigDecimal(key.substring(0, key.length() - 1)); | ||
| double weight = percentage.doubleValue(); | ||
| if (percentage.signum() <= 0 || !Double.isFinite(weight) || weight <= 0) { |
There was a problem hiding this comment.
Nit: the key regex [0-9]+(?:\.[0-9]+)?% already rules out NaN, Infinity and negative numbers, so only percentage.signum() <= 0 does anything here.
| } else { | ||
| throw invalid("each branch must be a string template or a non-empty list of string templates"); | ||
| } | ||
| if (!uniqueValues.add(qualified)) { |
There was a problem hiding this comment.
This uniqueness rule exists only because WeightedRandomSelector requires it. If select() uses the precomputed cumulative weights directly (see the comment on select), duplicate branch values become harmless and this check (and the matching sentence in the docs) can go.
| public Object fetch(String key, FakerContext context) { | ||
| List<?> valuesArray = null; | ||
| final Object o = fetchObject(key, context); | ||
| if (o instanceof WeightedYamlValues weighted) { |
There was a problem hiding this comment.
fetchObject is public and will now return a package-private WeightedYamlValues for weighted keys, which code outside the package can't use. For example, Unique.fetchFromYaml("weighted_custom.strings") throws "No values found". That's probably acceptable, but it should be a deliberate choice and mentioned in the docs.
Also, branch strings skip the /regex/ → #{regexify ...} conversion that safeFetch applies to ordinary values.
| - "10%": [".ee", ".lv", ".lt"] | ||
| ``` | ||
|
|
||
| Each entry must have exactly one quoted decimal percentage key and a string template or a non-empty list of string templates. Percentages must be finite, positive and total exactly 100 (decimal fractions such as `33.3%` are supported). Branch values must be unique, matching `WeightedRandomSelector`'s validation. Mixing weighted entries with ordinary entries, or nesting weighted choices inside a branch, is not supported. |
There was a problem hiding this comment.
Quoting isn't required: in YAML, - 60%: x already reads the key as a string. This section is also quite long; the last two paragraphs could be one sentence each.
| } | ||
|
|
||
| @Test | ||
| void sameSeedAndSharedYamlDoNotShareSamples() { |
There was a problem hiding this comment.
The name says "do not share samples", but the test asserts the two outputs are equal. Maybe sameSeedProducesSameSequence? Also, a few tests here write out java.util.List / java.util.Map in full; imports would be cleaner.
| } | ||
|
|
||
| @Test | ||
| void malformedCustomYamlFailsClearly() throws IOException { |
There was a problem hiding this comment.
Once the error message includes the key, it would be good to assert that here as well (e.g. hasMessageContaining("weighted_custom.value")).
|
I’ll calculate the weights once when the file loads, drop the |
Custom YAML files loaded with
addPathoraddUrlcan now give string or list choices a percentage. If a list has a 10% weight, Faker picks that list 10% of the time, then draws one of its values uniformly. This extends the Java selector from #1414 to the YAML use case in #1622.I used Codex to help implement and test this. The Linux Maven build passed on JDK 21, including Spotless and all 21,574 tests.